[Thread-Safety] Add Mutex per Interpreter #698

Vipul-Cariappa · 2025-08-11T12:50:56Z

Big changes in this PR. Some parts of the explanation might be like a conversation.

Mutex lock per interpreter.

Question: Do we need a per-interpreter lock?
Answer: Yes. Ideally, that would improve the performance in multi-threaded cases.

Question: Does the InterOp API fully support dispatching queries (some kind of lookup) or compiling code across multiple interpreters at the same time?
Answer: Actually, (I guess) No. Our API only supports stack-like access of multiple interpreters.
Example code that will not work:

auto interpreter_1 = Cpp::CreateInterpreter();
Cpp::Compile(R"(
void f() {}
)");
auto f = Cpp::GetNamed("f");

auto interpreter_2 = Cpp::CreateInterpreter();
Cpp::Compile(R"(
void ff() {}
)");

auto ff = Cpp::GetNamed("ff");

auto f_callable = Cpp::MakeFunctionCallable(f); // This fails because:
// MakeFunctionCallable uses the interpreter instance 2 at line
// https://github.com/compiler-research/CppInterOp/blob/dba60ff3d829551b1c86128593ca817af78d67dc/lib/CppInterOp/CppInterOp.cpp#L3148-L3150

But you can make the above example work, if the user rotates the sInterpreters.
Example:

auto interpreter_1 = Cpp::CreateInterpreter();
Cpp::Compile(R"(
void f() {}
)");
auto f = Cpp::GetNamed("f");

auto interpreter_2 = Cpp::CreateInterpreter();
Cpp::Compile(R"(
void ff() {}
)");

auto ff = Cpp::GetNamed("ff");

Cpp::ActivateInterpreter(interpreter_1); // look at https://github.com/compiler-research/CppInterOp/blob/dba60ff3d829551b1c86128593ca817af78d67dc/lib/CppInterOp/CppInterOp.cpp#L3273-L3287
auto f_callable = Cpp::MakeFunctionCallable(f);

Question: Ok. So is there a way for InterOp is maintain the rotation thing?
Answer: Would not be a easy thing to achieve. From my initial view on the matter, it might be possible. But a more important question; Should we support such usecases?

Question: Ok. So given the limitation of using multiple interpreter but only as a stack-access. Do we need a mutex lock per interpreter?
Answer: No, the user can only access the top most interpreter at a time. We don't need a mutex per interpreter. (Let me know if my reasoning is wrong somewhere)

Testing this by running in parallel

gtest does not support running in parallel (I could not find anything with my google search). But there is this project, gtest-parallel, that can split the tasks into separate isolated processes. But that is not what we want. We want to split the tests to run in parallel threads that share the same interpreter instance.

Code recovery RAII

@vgvassilev, I need some pointers on incorporating the codegen error recovery RAII. I don't think we should combine the two into the same class, but let me know your opinion. And how should I go about doing it?

codecov · 2025-08-11T13:00:08Z

Codecov Report

❌ Patch coverage is 97.22222% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.46%. Comparing base (33a3e7e) to head (d55415e).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/CppInterOp/CppInterOp.cpp	97.22%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
+ Coverage   79.83%   81.46%   +1.62%     
==========================================
  Files           9        9              
  Lines        3962     4213     +251     
==========================================
+ Hits         3163     3432     +269     
+ Misses        799      781      -18

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`95.55% <ø> (ø)`
lib/CppInterOp/CppInterOpInterpreter.h	`88.02% <ø> (-0.53%)`	⬇️
lib/CppInterOp/CppInterOp.cpp	`89.60% <97.22%> (+1.56%)`	⬆️

... and 2 files with indirect coverage changes

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`95.55% <ø> (ø)`
lib/CppInterOp/CppInterOpInterpreter.h	`88.02% <ø> (-0.53%)`	⬇️
lib/CppInterOp/CppInterOp.cpp	`89.60% <97.22%> (+1.56%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 13. Check the log or trigger a new build to see more.

lib/CppInterOp/CppInterOp.cpp

include/CppInterOp/CppInterOp.h

vgvassilev · 2025-08-12T05:11:22Z

lib/CppInterOp/CppInterOp.cpp

@@ -224,11 +301,18 @@ std::string Demangle(const std::string& mangled_name) {
  return demangle;
 }

-void EnableDebugOutput(bool value /* =true*/) { llvm::DebugFlag = value; }
+void EnableDebugOutput(bool value /* =true*/) {
+  LOCK(getInterpInfo());


I had a different mental model for this. I was thinking that we can have a StateMutatingSection RAII object which in turn has a mutex-based locking (if multithreading was enabled) and a 'PushTransactionRAII` (for pch/modules). The object should take a parameter if the change is in clang AST which might trigger a derserialiazation or llvm-only. In the latter case we can skip pushing a transaction.

There will be cases where we need one of the two concepts and not both. We need to design a flexible solution addressing these needs without having to introduce both concepts at the same time when they are not needed.

lib/CppInterOp/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

lib/CppInterOp/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 19. Check the log or trigger a new build to see more.

lib/CppInterOp/CppInterOp.cpp

unittests/CppInterOp/InterpreterTest.cpp

github-actions

clang-tidy made some suggestions

lib/CppInterOp/CppInterOp.cpp

unittests/CppInterOp/InterpreterTest.cpp

mcbarton · 2025-08-13T15:27:39Z

A note for this PR. The Emscripten build of CppInterOp doesn't currently support threads

CppInterOp/Emscripten-build-instructions.md

Line 95 in 6cf1c06

-DLLVM_ENABLE_THREADS=OFF \

It is possible to turns threads on, and modify CppInterOp, so that it builds with threads. I did this locally yesterday (I did this in the past before we had tests). The tests can run, but several of our currently passing tests fail with threads enabled. We also get a warning about shared memory for the passing tests. I can put a PR showing my current progress, but I don't know how far I would get enabling past what I already have since shared library with pthreads is labelled as experimental on Emscripten, so we might be trying to fight possible bugs in Emscripten.

unittests/CppInterOp/InterpreterTest.cpp

lib/.clang-tidy

mcbarton · 2025-08-14T15:26:08Z

Codecov Report

❌ Patch coverage is 94.02985% with 12 lines in your changes missing coverage. Please review. ✅ Project coverage is 80.34%. Comparing base (6cf1c06) to head (e6a33f8).
Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 94.02% 12 Missing ⚠️
Additional details and impacted files

🚀 New features to boost your workflow:

@Vipul-Cariappa can you add tests so that all lines of your patch are covered by tests?

github-actions

clang-tidy made some suggestions

lib/CppInterOp/CppInterOp.cpp

mcbarton · 2025-09-05T14:34:15Z

Codecov Report

❌ Patch coverage is 89.45869% with 37 lines in your changes missing coverage. Please review. ✅ Project coverage is 80.29%. Comparing base (d85eec0) to head (3cf9aec).
Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 89.45% 37 Missing ⚠️
Additional details and impacted files

🚀 New features to boost your workflow:

@vgvassilev Just over 10% over this PR is not covered by tests according to this message. Maybe @Vipul-Cariappa could add more tests to increase this coverage too before this goes in?

vgvassilev · 2025-09-05T14:46:48Z

Codecov Report

❌ Patch coverage is 89.45869% with 37 lines in your changes missing coverage. Please review. ✅ Project coverage is 80.29%. Comparing base (d85eec0) to head (3cf9aec).
Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 89.45% 37 Missing ⚠️
Additional details and impacted files
🚀 New features to boost your workflow:

@vgvassilev Just over 10% over this PR is not covered by tests according to this message. Maybe @Vipul-Cariappa could add more tests to increase this coverage too before this goes in?

Good point. I’d guess most of the uncovered lines are due to preexisting lack of coverage but would be nice to improve the coverage where possible.

github-actions

clang-tidy made some suggestions

lib/CppInterOp/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp

Vipul-Cariappa · 2025-09-08T10:05:52Z

Hi @mcbarton, I am using threads with that new test case I have written. And that test case is failing on WASM. I can just skip it. But was thinking if this can potentially enable it? Any comments or thoughts?

@vgvassilev, @mcbarton, The test I have written covers some more lines, but the effort to cover all of the missing lines isn't worth it. I advise we go forward with this. We can gradually cover more lines.

vgvassilev · 2025-09-08T10:16:28Z

Hi @mcbarton, I am using threads with that new test case I have written. And that test case is failing on WASM. I can just skip it. But was thinking if this can potentially enable it? Any comments or thoughts?

@vgvassilev, @mcbarton, The test I have written covers some more lines, but the effort to cover all of the missing lines isn't worth it. I advise we go forward with this. We can gradually cover more lines.

Disabling the wasm test should be fine.

I looked at the missing coverage. Yes, some are not caused by this PR but most are really trivial to add tests for and some are just dead code.

mcbarton · 2025-09-08T10:33:01Z

@Vipul-Cariappa Disabling it is the way to go. We do not currently support threads for the wasm build, and enabling them will quite an involved project from my investigations so far. My progress so far managed to get the tests to run with threads, but we end up with tests failing which currently pass. Also using threads with an Emscripten shared library is currently labelled as experimental.

Vipul-Cariappa · 2025-09-08T10:39:29Z

Skipped the test in WASM.
@vgvassilev, I guess this can be approved now. To be merged once the CI is green.

mcbarton · 2025-09-08T10:45:04Z

Skipped the test in WASM. @vgvassilev, I guess this can be approved now. To be merged once the CI is green.

I don't know how many commits this is behind main, but it needs rebasing according to the Github interface so we can ci can test your changes with the latest changes on main.

vgvassilev · 2025-09-08T11:07:32Z

Skipped the test in WASM. @vgvassilev, I guess this can be approved now. To be merged once the CI is green.

You don’t want to spend 30 mins writing these tests to make our coverage happy?

mcbarton · 2025-09-08T13:13:59Z

The tests which use printf (or anything which tries to write to standard output) will cause the ci to fail on Windows. This is a known issue and these tests will need to be skipped on Windows.

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/InterpreterTest.cpp

github-actions · 2025-09-08T13:17:53Z

unittests/CppInterOp/InterpreterTest.cpp

+    EXPECT_EQ(members.size(), 0);
+
+    EXPECT_FALSE(
+        Cpp::InsertOrReplaceJitSymbol("printf", (uint64_t)&printf_jit, I));


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

Cpp::InsertOrReplaceJitSymbol("printf", (uint64_t)&printf_jit, I)); ^

github-actions · 2025-09-08T13:17:54Z

unittests/CppInterOp/InterpreterTest.cpp

+    EXPECT_EQ(members.size(), 0);
+
+    EXPECT_FALSE(
+        Cpp::InsertOrReplaceJitSymbol("printf", (uint64_t)&printf_jit, I));


warning: no header providing "uint64_t" is directly included [misc-include-cleaner]

Cpp::InsertOrReplaceJitSymbol("printf", (uint64_t)&printf_jit, I)); ^

github-actions · 2025-09-08T13:17:54Z

unittests/CppInterOp/InterpreterTest.cpp

+    EXPECT_EQ(f_callable.getKind(), Cpp::JitCall::Kind::kGenericCall);
+
+    EXPECT_FALSE(
+        Cpp::TakeInterpreter((TInterp_t)1)); // try to take ownership of an


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

Cpp::TakeInterpreter((TInterp_t)1)); // try to take ownership of an ^

github-actions · 2025-09-08T13:17:54Z

unittests/CppInterOp/InterpreterTest.cpp

+    EXPECT_NE(std::find(includes.begin(), includes.end(), "/non/existent/"),
+              std::end(includes));
+
+    EXPECT_TRUE(Cpp::InsertOrReplaceJitSymbol("non_existent", (uint64_t)&f, I));


warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

EXPECT_TRUE(Cpp::InsertOrReplaceJitSymbol("non_existent", (uint64_t)&f, I)); ^

unittests/CppInterOp/TestSharedLib2/TestSharedLib.h

github-actions

clang-tidy made some suggestions

github-actions · 2025-09-08T13:55:03Z

unittests/CppInterOp/InterpreterTest.cpp

-  EXPECT_TRUE(Cpp::GetInterpreter()) << "External Interpreter not set";
+  Cpp::UseExternalInterpreter(ExtInterp);
+  EXPECT_EQ(ExtInterp, Cpp::GetInterpreter());
+  EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp));

 #ifndef CPPINTEROP_USE_CLING
  I.release();


warning: Method called on moved-from object 'I' [clang-analyzer-cplusplus.Move]

I.release(); ^

Additional context

unittests/CppInterOp/InterpreterTest.cpp:322: Assuming the condition is false

if (llvm::sys::RunningOnValgrind()) ^

unittests/CppInterOp/InterpreterTest.cpp:322: Taking false branch

if (llvm::sys::RunningOnValgrind()) ^

unittests/CppInterOp/InterpreterTest.cpp:338: Object 'I' is moved

auto CPPI = Cpp::Interpreter(std::move(I)); ^

unittests/CppInterOp/InterpreterTest.cpp:354: Control jumps to 'case 0:' at line 355

EXPECT_NE(ExtInterp, nullptr); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1886: expanded from macro 'EXPECT_NE'

EXPECT_PRED_FORMAT2(::testing::internal::CmpHelperNE, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:77: expanded from macro 'GTEST_ASSERT_'

GTEST_AMBIGUOUS_ELSE_BLOCKER_ \ ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-port.h:727: expanded from macro 'GTEST_AMBIGUOUS_ELSE_BLOCKER_'

switch (0) \ ^

unittests/CppInterOp/InterpreterTest.cpp:354: Assuming the condition is true

EXPECT_NE(ExtInterp, nullptr); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1886: expanded from macro 'EXPECT_NE'

EXPECT_PRED_FORMAT2(::testing::internal::CmpHelperNE, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:354: Taking true branch

EXPECT_NE(ExtInterp, nullptr); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1886: expanded from macro 'EXPECT_NE'

EXPECT_PRED_FORMAT2(::testing::internal::CmpHelperNE, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:357: Control jumps to 'case 0:' at line 358

EXPECT_EQ(ExtInterp, Cpp::GetInterpreter()); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:77: expanded from macro 'GTEST_ASSERT_'

GTEST_AMBIGUOUS_ELSE_BLOCKER_ \ ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-port.h:727: expanded from macro 'GTEST_AMBIGUOUS_ELSE_BLOCKER_'

switch (0) \ ^

unittests/CppInterOp/InterpreterTest.cpp:357: Assuming the condition is true

EXPECT_EQ(ExtInterp, Cpp::GetInterpreter()); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:357: Taking true branch

EXPECT_EQ(ExtInterp, Cpp::GetInterpreter()); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:358: Control jumps to 'case 0:' at line 359

EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp)); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:77: expanded from macro 'GTEST_ASSERT_'

GTEST_AMBIGUOUS_ELSE_BLOCKER_ \ ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-port.h:727: expanded from macro 'GTEST_AMBIGUOUS_ELSE_BLOCKER_'

switch (0) \ ^

unittests/CppInterOp/InterpreterTest.cpp:358: Assuming the condition is true

EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp)); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:358: Taking true branch

EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp)); ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:1884: expanded from macro 'EXPECT_EQ'

EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:143: expanded from macro 'EXPECT_PRED_FORMAT2'

GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:133: expanded from macro 'GTEST_PRED_FORMAT2_'

GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), on_failure) ^

build/unittests/googletest-prefix/src/googletest/googletest/include/gtest/gtest_pred_impl.h:78: expanded from macro 'GTEST_ASSERT_'

if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^

unittests/CppInterOp/InterpreterTest.cpp:361: Method called on moved-from object 'I'

I.release(); ^

github-actions · 2025-09-08T13:55:03Z

unittests/CppInterOp/TestSharedLib2/TestSharedLib.h

@@ -0,0 +1,11 @@
+#ifndef UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H
+#define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H


warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change

#define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H

#ifndef GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTSHAREDLIB2_TESTSHAREDLIB_H

#define GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTSHAREDLIB2_TESTSHAREDLIB_H

unittests/CppInterOp/TestSharedLib2/TestSharedLib.h:10:

- #endif // UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H + #endif // GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTSHAREDLIB2_TESTSHAREDLIB_H

vgvassilev

Well done!

LGTM, modulo the comments. Consider squashing all into one commit or condensing a good atomic minimal set of commits...

vgvassilev · 2025-09-08T14:05:21Z

lib/CppInterOp/CppInterOp.cpp

 }

 std::string ObjToString(const char* type, void* obj) {
-  return getInterp().toString(type, obj);
+  LOCK(getInterpInfo(NULLPTR)); // FIXME: not enough information to lock the
+                                // corrent interpreter


Suggested change

// corrent interpreter

// current interpreter

Please move the comment above the line.

Have a mutex for each interpreter instance create. Only use the lock at writes to the AST. Use hashmap to identify Sema & AST from individual Decl We need to identify which interpreter a Decl belongs to, when using multiple interpreter. We do it by checking which `clang::ASTContext` the `clang::Decl` belongs We maintain a map: `clang::ASTContext -> Cpp::InterpreterInfo`. Using this map, we identify the correct interpreter. Note that the mutex is stored in the InterpreterInfo class. There are 2 usecases for this: 1. We can now lock the correct interpreter making it thread safe. 2. User of `libCppInterOp` need not set the correct active interpreter using `Cpp::ActivateInterpreter`, this information can be retrived using the map.

Pops interpreter without destroying it. Destruction to be handled by the caller. This is to be used along with UserExternalInterpreter, where libCppInterOp is not owning the interpreter instance.

Functions like Declare compile code into the top-most interpreter in the stack. If the user wants to compile the code to some other interpreter instance, user can use the `TInterp_t` argument to specify the interpreter. This is also required to lock the correct interpreter instance. If `TInterp_t` is null, we assume the correct interpreter to lock is the top of the stack. This assumption might be wrong at some cases.

Add test to run things in parallel Add documentation for developers

Vipul-Cariappa requested review from vgvassilev, aaronj0, anutosh491 and mcbarton August 11, 2025 12:50

Vipul-Cariappa marked this pull request as draft August 11, 2025 12:51

github-actions bot reviewed Aug 11, 2025

View reviewed changes

Vipul-Cariappa commented Aug 11, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

Vipul-Cariappa marked this pull request as ready for review August 11, 2025 13:29

Vipul-Cariappa commented Aug 11, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

vgvassilev reviewed Aug 12, 2025

View reviewed changes

Vipul-Cariappa force-pushed the dev/lock branch from b689053 to 7659fcb Compare August 12, 2025 09:05

github-actions bot reviewed Aug 12, 2025

View reviewed changes

Vipul-Cariappa force-pushed the dev/lock branch from 7659fcb to 46697ce Compare August 13, 2025 11:27

github-actions bot reviewed Aug 13, 2025

View reviewed changes

vgvassilev reviewed Aug 13, 2025

View reviewed changes

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

Vipul-Cariappa mentioned this pull request Aug 13, 2025

Mutex lock (implementation base on ASTContext::getAllocator::identifyObject) #704

Draft

Vipul-Cariappa force-pushed the dev/lock branch from 46697ce to 68d616d Compare August 13, 2025 13:43

github-actions bot reviewed Aug 13, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved

mcbarton reviewed Aug 13, 2025

View reviewed changes

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

Vipul-Cariappa force-pushed the dev/lock branch from 68d616d to 47d1f58 Compare August 14, 2025 07:53

vgvassilev reviewed Aug 14, 2025

View reviewed changes

lib/.clang-tidy Outdated Show resolved Hide resolved

Vipul-Cariappa force-pushed the dev/lock branch from 47d1f58 to e6a33f8 Compare August 14, 2025 08:16

Vipul-Cariappa force-pushed the dev/lock branch 3 times, most recently from 57caa27 to 97bd9b8 Compare August 22, 2025 11:34

github-actions bot reviewed Aug 22, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Show resolved Hide resolved

Vipul-Cariappa changed the title ~~add mutex per interpreter per thread~~ [Thread-Safety] Add Mutex per Interpreter Aug 27, 2025

Vipul-Cariappa force-pushed the dev/lock branch from 3cf9aec to 2ed0dfe Compare September 5, 2025 14:45

Vipul-Cariappa force-pushed the dev/lock branch 2 times, most recently from 4feb220 to 1e473ce Compare September 5, 2025 15:14

github-actions bot reviewed Sep 5, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

Vipul-Cariappa force-pushed the dev/lock branch from 1e473ce to ab26665 Compare September 8, 2025 09:39

github-actions bot reviewed Sep 8, 2025

View reviewed changes

unittests/CppInterOp/InterpreterTest.cpp Show resolved Hide resolved

vgvassilev force-pushed the dev/lock branch from ab26665 to 84e70b0 Compare September 8, 2025 10:11

Vipul-Cariappa force-pushed the dev/lock branch from 84e70b0 to 0afd579 Compare September 8, 2025 10:37

Vipul-Cariappa force-pushed the dev/lock branch from 0afd579 to 61e0584 Compare September 8, 2025 10:47

Vipul-Cariappa force-pushed the dev/lock branch from 61e0584 to c36be86 Compare September 8, 2025 13:05

github-actions bot reviewed Sep 8, 2025

View reviewed changes

Vipul-Cariappa force-pushed the dev/lock branch from c36be86 to bc7fac3 Compare September 8, 2025 13:42

github-actions bot reviewed Sep 8, 2025

View reviewed changes

vgvassilev approved these changes Sep 8, 2025

View reviewed changes

Vipul-Cariappa force-pushed the dev/lock branch from bc7fac3 to ab93975 Compare September 10, 2025 08:08

Vipul-Cariappa added 4 commits September 10, 2025 10:12

Add Cpp::TakeInterpreter

95c85ae

Pops interpreter without destroying it. Destruction to be handled by the caller. This is to be used along with UserExternalInterpreter, where libCppInterOp is not owning the interpreter instance.

Add tests and dev-doc

d55415e

Add test to run things in parallel Add documentation for developers

Vipul-Cariappa force-pushed the dev/lock branch from ab93975 to d55415e Compare September 10, 2025 08:21

Vipul-Cariappa merged commit c42c907 into compiler-research:main Sep 10, 2025
62 of 63 checks passed

		@@ -0,0 +1,11 @@
		#ifndef UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H
		#define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H

	#define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H
	#ifndef GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTSHAREDLIB2_TESTSHAREDLIB_H
	#define GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTSHAREDLIB2_TESTSHAREDLIB_H

[Thread-Safety] Add Mutex per Interpreter #698

[Thread-Safety] Add Mutex per Interpreter #698

Uh oh!

Conversation

Vipul-Cariappa commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mutex lock per interpreter.

Testing this by running in parallel

Code recovery RAII

Uh oh!

codecov bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vgvassilev Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcbarton commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

mcbarton commented Aug 14, 2025

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Vipul-Cariappa commented Aug 11, 2025 •

edited

Loading

codecov bot commented Aug 11, 2025 •

edited

Loading

mcbarton commented Sep 8, 2025 •

edited

Loading

mcbarton commented Sep 8, 2025 •

edited

Loading